-
Notifications
You must be signed in to change notification settings - Fork 774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Tab view on settings page #3551
Add Tab view on settings page #3551
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gautamjajoo minor change required,
rest LGTM 👍
frontend_v2/src/app/components/challenge/challengesettings/challengesettings.component.html
Outdated
Show resolved
Hide resolved
@Kajol-Kumari Done! |
|
frontend_v2/src/app/components/challenge/challengesettings/challengesettings.component.html
Outdated
Show resolved
Hide resolved
frontend_v2/src/app/components/challenge/challengesettings/challengesettings.component.html
Outdated
Show resolved
Hide resolved
frontend_v2/src/app/components/challenge/challengesettings/challengesettings.component.html
Show resolved
Hide resolved
frontend_v2/src/app/components/challenge/challengesettings/challengesettings.component.html
Show resolved
Hide resolved
frontend_v2/src/app/components/challenge/challengesettings/challengesettings.component.html
Show resolved
Hide resolved
frontend_v2/src/app/components/challenge/challengesettings/challengesettings.component.html
Show resolved
Hide resolved
frontend_v2/src/app/components/challenge/challengesettings/challengesettings.component.html
Outdated
Show resolved
Hide resolved
frontend_v2/src/app/components/challenge/challengesettings/challengesettings.component.html
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gautamjajoo can we please fix the below minor responsiveness issues -
- The private tag for phase select box shouldn't get hidden for small screen. ref ⬇️
- The left-padding for
Evaluation
tab goes off for screens smaller than 700px. We should keep it consistent throughout. ref ⬇️
|
@Kajol-Kumari Updated the PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gautamjajoo The padding on the settings page doesn't look consistent with the other pages, please fix it. See the ref image below -
@@ -28,6 +28,17 @@ | |||
padding-right: 0px; | |||
} | |||
|
|||
@media (max-width: 400px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it till 800px
as the private-tag issue is for ipad sized screens as well
@Kajol-Kumari In the above screenshot is it regarding the space between title and the tabs? Because it was intentional as they were tabs so I thought some space would be needed? |
I am taking about the top-padding for the title. Here the top-padding is larges as compared with the other tabs. Also the spacing btw title and tabs can also be reduced a bit.(we can keep it same as the top padding but the top padding should be same as other tabs) |
@Kajol-Kumari Done the changes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the requested changes @gautamjajoo
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gautamjajoo can we add heading Evaluation script
on he Evaluation tab
@Ram81 Heading of the first card where evaluation script and criteria is present? |
Yes |
This PR:
@Ram81 @Kajol-Kumari
tab-view.mp4